Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move stats to metrics #241

Merged
merged 1 commit into from
May 10, 2019
Merged

Conversation

bogdandrutu
Copy link
Member

The main idea is to reduce confusion about why two top level APIs exists to record metrics/stats/measurement.

We still need to offer the ability to record raw measurements as well as pre-defined metrics (including aggregation and labels).

The structure of the Metrics package is not final:

Do we need MetricsRegistry or we move all the methods to Meter? The SDK will allow users to set Filters that apply to all created metrics (similar with metric registry in Micrometer for example).
The Views API in the SDK is not yet moved to metrics.

This PR is based on #226 which was started from a branch that lived in the main repo because that was under my account. We will move all the discussions here.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented May 2, 2019

@yurishkuro #226 (comment):
Some questions/comments:

  • in many other metrics APIs that allow recording gauges, timers, and/or histograms, the recording API is still about recording the raw measure, but behind the scene the library may or may not perform some form of aggregation, like LastValue for gauges, or Histogram for timers. So the distinction between raw measure and Gauge/Histogram is not clear to me, since from instrumentation point of view it's always recording the raw value.
  • on the other hand, counters seem to be fundamentally different and can never be captured via raw measure, is that correct? This breaks the mental model for me.
  • being able to change aggregation without touching instrumentation is great - I often wanted to see something like queue_length not as a gauge but as a histogram (incidentally tally didn't support histograms for non-timer values)
  • application of request-scoped tags is also very tricky. As @bogdandrutu said, there are some stats that don't make sense to be labeled with request-scoped tags, e.g. CPU utilization. For measures where we do want to apply context tags, the ability to define views also provides a place to define context tag rules, potentially on a per-measure basis, e.g. "allow tags A and B for measureX, but only tag B for measure Y" (however, see next item).
  • the requirement to define views seems very burdensome, especially because they need to reference the actual measure objects. In a complex, multi-layer system like Jaeger backend, the metrics defined by the individual components are usually encapsulated within those components, and it's not feasible to have a single place in the app where I can define views for all measures from all components. On the other hand, if views are defined internally by the components themselves, then it mostly makes the views redundant, since the components can define aggregations at the time of constructing the measures, which is what typically happens with most other metrics APIs where you explicitly ask for Counter, Gauge, Histogram. I'd be interested to see how more complex systems using OC deal with this encapsulation issue.

@SergeyKanzhelev
Copy link
Member

  • in many other metrics APIs that allow recording gauges, timers, and/or histograms, the recording API is still about recording the raw measure, but behind the scene the library may or may not perform some form of aggregation, like LastValue for gauges, or Histogram for timers. So the distinction between raw measure and Gauge/Histogram is not clear to me, since from instrumentation point of view it's always recording the raw value.
  • on the other hand, counters seem to be fundamentally different and can never be captured via raw measure, is that correct? This breaks the mental model for me.

So this PR doesn't make anything worse, correct? We might need an issue for this. It is a good property to be able to express the SpanData of metrics. Basically read from file and proxy the metric.

  • application of request-scoped tags is also very tricky. As @bogdandrutu said, there are some stats that don't make sense to be labeled with request-scoped tags, e.g. CPU utilization. For measures where we do want to apply context tags, the ability to define views also provides a place to define context tag rules, potentially on a per-measure basis, e.g. "allow tags A and B for measureX, but only tag B for measure Y" (however, see next item).
  • the requirement to define views seems very burdensome, especially because they need to reference the actual measure objects. In a complex, multi-layer system like Jaeger backend, the metrics defined by the individual components are usually encapsulated within those components, and it's not feasible to have a single place in the app where I can define views for all measures from all components. On the other hand, if views are defined internally by the components themselves, then it mostly makes the views redundant, since the components can define aggregations at the time of constructing the measures, which is what typically happens with most other metrics APIs where you explicitly ask for Counter, Gauge, Histogram. I'd be interested to see how more complex systems using OC deal with this encapsulation issue.

Couple of thoughts - I think tags in views is an extremely powerful tool for customer defined tags. Furthermore for some metrics like http.response_time - they should be associated with span itself (or parent span or root span), rather than with the tags that supposed to be set on the context. Tags may be left for the customer defined properties.

CPU metrics will never be reported from the thread that has tags. So there will never be a case when request properties got applied to global CPU perf counter.

Again, perhaps we need to start filing individual issues for this feedback, as it touches some interesting points.

@@ -23,7 +23,7 @@
import javax.annotation.concurrent.Immutable;
import openconsensus.internal.StringUtils;
import openconsensus.internal.Utils;
import openconsensus.stats.Measure;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you kept tests namespace for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the SDK, I did not change that for the moment.

@bogdandrutu bogdandrutu force-pushed the rmstats branch 2 times, most recently from 8f909ca to 23c8f3e Compare May 6, 2019 21:48
@bogdandrutu bogdandrutu merged commit 67f5523 into open-telemetry:master May 10, 2019
@bogdandrutu bogdandrutu deleted the rmstats branch May 10, 2019 23:50
@yurishkuro
Copy link
Member

@SergeyKanzhelev I broke my comments into two issues: #268 and #269.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants